[linter-miner] linter: add sprintfint — flag fmt.Sprintf("%d", x) where x is int - #42538
Conversation
Adds a new custom Go analysis linter, `sprintfint`, that reports calls of
the form `fmt.Sprintf("%d", x)` where `x` is a single value of type `int`.
These can be rewritten as `strconv.Itoa(x)`, which is simpler, avoids
format-string parsing at runtime, and makes the intent clearer.
## What it catches
```go
// flagged
s := fmt.Sprintf("%d", n) // n is int
// preferred
s := strconv.Itoa(n)
```
## What it does NOT flag
- `fmt.Sprintf("%d", n64)` — n64 is int64 (use strconv.FormatInt)
- `fmt.Sprintf("%d", u)` — u is uint
- `fmt.Sprintf("%d %d", a, b)` — multiple verbs
- Calls suppressed with `//nolint:sprintfint`
## Implementation
- `pkg/linters/sprintfint/sprintfint.go` — analyzer
- `pkg/linters/sprintfint/sprintfint_test.go` — tests via analysistest
- `pkg/linters/sprintfint/testdata/src/sprintfint/sprintfint.go` — fixtures
- `cmd/linters/main.go` — registration in multichecker
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills... |
|
✅ Test Quality Sentinel completed test quality analysis. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (174 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Review: sprintfint linter
Good concept and clean structure. One correctness bug needs fixing before merge.
Blocking — correctness bug
argType.Underlying() matches named int types (sprintfint.go:77)
The check argType.Underlying() != types.Typ[types.Int] passes for any type whose underlying type is int, including user-defined named types like type Status int or type Counter int. The resulting SuggestedFix emits strconv.Itoa(s) which fails to compile when s is a named type — strconv.Itoa only accepts the exact predeclared int.
Fix: remove .Underlying():
if argType != types.Typ[types.Int] {
return
}Add a goodNamedInt fixture to testdata to prevent regression.
Non-blocking
main.gofunction body:sprintfint.Analyzeris registered beforesprintferrorsnew.Analyzer, but alphabetical order (sprintferr...<sprintfi...) requires it to come after.buildItoaFixdoes not managestrconvimport addition or potentialfmtimport removal; advisory for tooling authors applying this fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 70.1 AIC · ⌖ 11.5 AIC · ⊞ 4.9K
| if argType == nil { | ||
| return | ||
| } | ||
| if argType.Underlying() != types.Typ[types.Int] { |
There was a problem hiding this comment.
Bug: Underlying() causes false positives for named types based on int
The comment on line 71 says "exact type int", but argType.Underlying() also matches any named type whose underlying type is int, e.g.:
type Status int
var s Status = 5
fmt.Sprintf("%d", s) // ← flagged by this linterThe suggested fix strconv.Itoa(s) fails to compile because strconv.Itoa only accepts int, not Status. Use exact type equality instead:
if argType != types.Typ[types.Int] {
return
}This limits the linter to the exact predeclared int type and avoids broken auto-fix suggestions for named int types.
@copilot please address this.
| seenmapbool.Analyzer, | ||
| sortslice.Analyzer, | ||
| sprintferrdot.Analyzer, | ||
| sprintfint.Analyzer, |
There was a problem hiding this comment.
Minor: analyzer registration is out of alphabetical order
The imports (lines 47–49) are correctly sorted: sprintferrdot → sprintferrorsnew → sprintfint. But the registration in main() has sprintfint.Analyzer inserted before sprintferrorsnew.Analyzer, which reverses the correct order (sprintferr... < sprintfi... lexicographically).
Expected order:
sprintferrdot.Analyzer,
sprintferrorsnew.Analyzer,
sprintfint.Analyzer,@copilot please address this.
| // suppressed suppresses the linter directive — no diagnostic expected. | ||
| func suppressed(n int) string { | ||
| return fmt.Sprintf("%d", n) //nolint:sprintfint | ||
| } |
There was a problem hiding this comment.
Missing test case: named type based on int
After fixing the Underlying() type check, add a test fixture that confirms named int types are not flagged:
// goodNamedInt uses a named type based on int — not flagged, because
// strconv.Itoa does not accept named types.
func goodNamedInt(n Counter) string {
type Counter int // or defined at package level
return fmt.Sprintf("%d", n)
}Without this, a future refactor that reintroduces Underlying() could silently re-break the behaviour.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Test coverage breakdown —
Verdict
References: §28467215764
|
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness defects in the analyzer + one fix completeness gap
Blocking issues found (fix required before merge)
-
Broken
SuggestedFixfor named int types (sprintfint.go:77):argType.Underlying()causes the linter to fire ontype MyInt intarguments, but the emitted fixstrconv.Itoa(x)does not compile for named types. Change toargType != types.Typ[types.Int](direct comparison) so only bareintis matched, or generatestrconv.Itoa(int(x))for named types. -
SuggestedFixdoesn't add thestrconvimport (sprintfint.go:105): applying the fix in a file that doesn't already import"strconv"produces uncompilable code. Either handle the importTextEditor document the limitation explicitly. -
Out-of-order Analyzer registration (
cmd/linters/main.go:89):sprintfint.Analyzeris inserted betweensprintferrdotandsprintferrorsnew; alphabetically it belongs aftersprintferrorsnew. -
Missing named-type test fixture (
testdata/.../sprintfint.go:48): no test guards thetype MyInt intboundary, so the false-positive + broken-fix scenario goes undetected.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 117.1 AIC · ⌖ 8.13 AIC · ⊞ 1.6K
Comment /review to run again
| if argType == nil { | ||
| return | ||
| } | ||
| if argType.Underlying() != types.Typ[types.Int] { |
There was a problem hiding this comment.
Underlying() match fires on named int types but generates uncompilable SuggestedFix: type MyInt int; fmt.Sprintf("%d", myIntVar) passes this guard (because MyInt.Underlying() == types.Typ[types.Int]), but the emitted fix strconv.Itoa(myIntVar) does not compile—strconv.Itoa only accepts bare int, not a named type.
💡 Suggested fix
Use a direct pointer comparison so only the bare int type is matched:
// Before — incorrectly matches named types derived from int
if argType.Underlying() != types.Typ[types.Int] {
return
}
// After — only matches the exact built-in int type
if argType != types.Typ[types.Int] {
return
}If named int types should also be flagged in the future, the fix generator must emit strconv.Itoa(int(x)) (with the explicit conversion), not just strconv.Itoa(x).
| { | ||
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| NewText: []byte("strconv.Itoa(" + argText + ")"), |
There was a problem hiding this comment.
SuggestedFix omits the strconv import, producing uncompilable code when applied in files that don't already import it: the TextEdit only rewrites the call site. Any file where "strconv" is not yet imported will fail to compile after the fix is applied.
💡 Suggested fix
Check whether strconv is already imported and add a TextEdit for the import declaration when it is not. Other analyzers in this repo (e.g. fprintlnsprintf) handle this via a helper that inserts the import block edit.
Alternatively, document the limitation clearly in Analyzer.Doc so that tool integrators know goimports / gopls must be run alongside the fix. As-is, a user running go vet -fix will silently get broken code.
| seenmapbool.Analyzer, | ||
| sortslice.Analyzer, | ||
| sprintferrdot.Analyzer, | ||
| sprintfint.Analyzer, |
There was a problem hiding this comment.
sprintfint.Analyzer is registered out of alphabetical order: it appears between sprintferrdot and sprintferrorsnew, but alphabetically sprintferr* < sprintfint (since 'e' < 'i'). The import block in this same file is in the correct order (sprintferrorsnew → sprintfint → ssljson); the main() slice should match.
💡 Suggested fix
// in main()
sprintferrdot.Analyzer,
sprintferrorsnew.Analyzer, // move before sprintfint
sprintfint.Analyzer,| // suppressed suppresses the linter directive — no diagnostic expected. | ||
| func suppressed(n int) string { | ||
| return fmt.Sprintf("%d", n) //nolint:sprintfint | ||
| } |
There was a problem hiding this comment.
Missing test case for named int types: the fixture has no case like type MyInt int; fmt.Sprintf("%d", myIntVar), which is the exact scenario where the Underlying()-based type check produces a false positive and the generated SuggestedFix emits uncompilable code. Add a // want -free case (or an explicit no-diagnostic marker) to catch regressions on this boundary once the type check is corrected.
💡 Suggested addition
type myInt int
// goodNamedInt is a named type; the linter must NOT flag this because the
// suggested fix strconv.Itoa(x) would not compile for a non-int type.
func goodNamedInt(n myInt) string {
return fmt.Sprintf("%d", n) // no diagnostic expected
}|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new custom Go static-analysis linter,
sprintfint, that flagsfmt.Sprintf("%d", x)calls wherexhas the exact predeclared typeintand suggests replacing them withstrconv.Itoa(x).What Changed
pkg/linters/sprintfint/sprintfint.gopkg/linters/sprintfint/sprintfint_test.goanalysistestpkg/linters/sprintfint/testdata/src/sprintfint/sprintfint.goanalysistestfixture with// wantannotationscmd/linters/main.gosprintfint.Analyzerin the multicheckerdocs/adr/42538-add-sprintfint-linter.mdDetection Logic
The analyzer walks
ast.CallExprnodes and reports a diagnostic when all of the following hold:fmt.Sprintf(selector check viaastutil.IsPkgSelector)."%d".types.Typ[types.Int](plainint; named types,int64,uint, etc. are not flagged).Skips
_test.gofiles (filecheck.IsTestFile) and respects//nolint:sprintfintline directives.Suggested Fix
buildItoaFixemits a singleanalysis.TextEditrewriting the full call expression tostrconv.Itoa(<arg>). Editors andgo toolcan auto-apply the fix; callers may needgoimportsto adjust imports afterward.Scope Boundaries
inttype is covered.int64,int32,uint, and named integer types are intentionally out of scope.Context (from ADR-42538)
Two existing production call sites (
pkg/stringutil/stringutil.go:59,pkg/console/console_wasm.go:41) use the suboptimalfmt.Sprintf("%d", x)idiom. This linter prevents regression after those sites are fixed. Alternatives considered: code-review-only enforcement (proven insufficient), externalperfsprintlinter (broader scope, breaks internal convention), and broader integer-type coverage (deferred due to type-specific suggestion complexity).Commits
18c8a69b4linter: add sprintfint — flag fmt.Sprintf("%d", x) where x is int52f92c3e0docs(adr): draft ADR-42538 for sprintfint linter addition0c271b511chore: start pr finisher pass02539014cfix sprintfint review feedback